Misc. Improvements Draft PR#2294
Conversation
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Thanks for submitting this, I think it'd make sense to split this into distinct PRs?
| .PHONY: bin | ||
| bin: manpages | ||
| cargo build --release --features "$(CARGO_FEATURES)" --bins | ||
| if [ ! -x "$(BOOTC_TARGET_DIR)/bootc" ]; then \ |
There was a problem hiding this comment.
This completely breaks the buildsystem, no? We'd just silently be reusing old binaries...
There was a problem hiding this comment.
yes, that patch is not good and not designed for merge. But it did its job for me.
It is very hard to iterate with bootc while having it compile three times to deploy
| .PHONY: manpages | ||
| manpages: | ||
| cargo run --release --package xtask -- manpages | ||
| if [ -x "$(BOOTC_TARGET_DIR)/bootc" ]; then \ |
There was a problem hiding this comment.
But yes, this is really really messy how we have our documentation depend on running the binary.
Maybe what is easier is to try to move instead to where we just validate the generated docs as a separate phase, but don't default to linking the two together.
It's effectively what we did for the tmt stuff
| let commit = commit.to_string(); | ||
| let ocidir = ociw.dir().try_clone().context("Cloning OCI directory")?; | ||
| let opts = worker_opts.clone(); | ||
| handles.push(thread::spawn(move || { |
There was a problem hiding this comment.
I think it'd be cleaner to use https://doc.rust-lang.org/stable/std/thread/fn.scope.html - or use rayon. (Or use tokio)
There was a problem hiding this comment.
Yeah, this was a fast fixup that works. I wouldn't be happy merging it, the deploy speedup is real though
| r.push(max_freq_components); | ||
| } | ||
|
|
||
| // Allocate an empty bin for new packages |
There was a problem hiding this comment.
I agree we never used this in practice, and we can probably drop it now. OTOH, this code is basically obsoleted by https://github.com/coreos/chunkah anyways?
There was a problem hiding this comment.
that is a nice tool. I will keep it in mind for non-ostree images
ostree-ext provides a fast deploy path for ostree backends so it will have to stay around until the composefs backend is stable with similar performance to the fast ostree deploy path, then add 2-3 more years for migration or a way to specify to bootc interim migration paths so e.g., version 2027.3 switches to composefs and points ostree instances to 2027.2 that has the machinery to migrate from ostree to composefs while being built for ostree. Ah.
| META_CONFIG, | ||
| import.config.to_canon_json_string()?.to_variant(), | ||
| ); | ||
| if let Some(version) = image_version { |
There was a problem hiding this comment.
Related but not quite. It's true that for images built with docker / buildah they need to be tagged twice so docker compatible registries that only read the leaf manifest can see the tag and tools like podman / skopeo that read the inner config can pick up those
This fixes the bootloader version exposed by ostree. Without this entries appear like PRETTY_NAME. This PR supersedes the one for PRETTY_BOOTLOADER
Ideally on the spinoff, bootc could be taught to set the version tag on the merge commit based on org.opencontainers.image.version while falling back to ostree
|
Hi, yes, this PR is not designed to merge It is more of an issue with some draft solutions attached, moreso for inspiration. Which ones do you think should spinoff as draft PRs and which should land in an issue? |
2 participants
Here are some improvements I came up with while working with bootc lately. They are by no means ready to merge (and vibed). Hopefully they are an inspiration.
Here is a small summary.
bootc internals ostree-ext container encapsulate --contentmetastill adds the empty package layer. It probably shouldn't. The caller can add an extra layer if they desire. I am unaware of someone using the previous manifest option with rpm-ostree in either case.bootc internals ostree-ext container encapsulateand ABI is still single threaded, and because of the current architecture it's very hard to fix (by hand). But it is around 3 times faster with 4 workers (~3min to 40s in testing)bootc internals ostree-ext container unencapsulatealways does a checkout, even when a commit has no derived layers. This adds 10-20 seconds on deployment times universally and spins fans, even though it is not needed. It is also important for my usecase because it means you cannot do unencapsulate on abare-split-attrsrepo currently, because it does not support checkouts.